Improvement: Support ZST in mamba and enable ZST by default #2404
Conversation
mamba/mamba/utils.py
Outdated
| @@ -274,6 +277,7 @@ def get_base_url(url, name=None): | |||
| api_ctx.retry_backoff = context.remote_backoff_factor | |||
| api_ctx.add_pip_as_python_dependency = context.add_pip_as_python_dependency | |||
| api_ctx.use_only_tar_bz2 = context.use_only_tar_bz2 | |||
| api_ctx.repodata_use_zst = True | |||
There was a problem hiding this comment.
I hard-coded all around to true, and I should obviously pass it along from the context.
What's your preference on the path forwards? Should i make repodata_use_zst configurable using an env var (a la MAMBA_REPODATA_USE_ZST), read that in here and set it accordingly?
|
I built this locally and verified that I now hit the .zst repodatas:
|
This reverts commit d73628b.
|
It seems like zstd support is pretty stable in Micromamba, do we just want to remove the |
|
I'm happy to repurpose this PR to do so if y'all are onboard 👍 |
I agree, we should use zstd everywhere and remove the option |
|
OK let's do it! |
|
Should fix this first #2418 |
|
Thanks for tackling this @johnhany97 @jonashaag #2418 should be fixed now |
|
@corneliusroemer do you want to complete this PR? Happy to merge when done |
|
I'm also happy to fix the broken test here. Can likely get to this later this week. |
johnhany97
changed the title
# Conflicts: # libmambapy/libmambapy/__init__.pyi
|
I've opened a separate PR to discuss the removal of the This PR for now assumes this flag is here to stay, and thus wires it up accordingly, if we end up merging the other PR first, then I can remove these flags in this PR. |
johnhany97
changed the title
|
Windows CI tasks seem to be failing with: which I believe is not relevant to this PR, but i'm not sure how to fix. |
|
The Windows failure is unrelated and a fix is proposed in #2526. |
| if info["needs_finalising"]: | ||
| sd.finalize_checks() |
There was a problem hiding this comment.
I'm not too familiar with this part of the code base, can you explain what is going on here? Are there any alternatives to having this logic here in Python?
There was a problem hiding this comment.
This is effectively just me mimicking the code that's in channel_loader which atm is duplicated between mamba and micromamba.
Let me know if you have an idea in mind that I could test out to make this nicer.
There was a problem hiding this comment.
More concretely though, what's happening in the zst-enabled codepaths is that we do a HEAD request first to check if the zst variant exists, and if it does, we add another target that's the actual GET request for the zst variant.
|
Looks like this is good to merge :D |
|
@JohanMabille should we proceed with merging this? |
|
Thank you @jonashaag 🚀 |
…amba-org#2404)" This reverts commit 60cd358.
…amba-org#2404)" This reverts commit 60cd358.
…amba-org#2404)" This reverts commit 60cd358.
|
@johnhany97 @jonashaag this PR was reverted in #2637. I t was likely merged out of date with |
|
Thanks for letting me know and sorry for the trouble. I should have rebased before merging because the PR was already pretty old. |
Before this PR
Only micromamba benefited from ZST support added in #2113
In addition, it was only enabled behind a flag, where users are required to add
repodata_use_zst: trueto their .condarc file.After this PR
mamba benefits from ZST support as well, and users no longer need to manually enable the flag. From now on, zst repodatas will be used where they're available.